-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add enable workflow trigger endpoint #6443
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
The pull request introduces a new endpoint to enable workflow triggers, along with several supporting changes to enhance error handling and integrate the new functionality into the existing system.
- Added
WorkflowTriggerModule
: Integrated intocore-engine.module.ts
to manage workflow triggers. - New Exception Handling: Introduced
workflow-trigger-graphql-api-exception-handler.util.ts
andworkflow-trigger.exception.ts
for specific error management. - GraphQL Mutation: Added
enableWorkflowTrigger
inworkflow-trigger.resolver.ts
for enabling triggers. - Service Implementation: Created
WorkflowTriggerService
inworkflow-trigger.service.ts
to handle trigger operations. - Entity Updates: Modified
workflow-version.workspace-entity.ts
and addedworkflow-event-listener.workspace-entity.ts
to support new trigger types and event listeners.
13 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this to a business module (except the resolver as we don't have the reflection yet)
...ges/twenty-server/src/modules/workflow/standard-objects/workflow-version.workspace-entity.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/workflow/workflow-trigger.service.ts
Outdated
Show resolved
Hide resolved
await workflowEventListenerRepository.delete({ | ||
workflowId, | ||
eventName, | ||
}); | ||
|
||
const workflowEventListener = await workflowEventListenerRepository.create({ | ||
workflowId, | ||
eventName, | ||
}); | ||
|
||
await workflowEventListenerRepository.save(workflowEventListener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should probably use workflowEventListenerRepository.upsert
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martmull unicity is not available for workspace entities yet. Adding a TODO to update that part!
) {} | ||
|
||
@Mutation(() => Boolean) | ||
async enableWorkflowTrigger( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enableWorkflowTrigger
-> I don't understand the naming here
triggerWorkflow
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martmull It does not really trigger the workflow. It setup the workflow trigger, so it will get triggered later.
I could rename it setupWorflowTrigger
but this is not that fare from enableWorkflowTrigger
. So for this one let's remain consistent with activePieces
? Wdyt?
return true; | ||
} | ||
|
||
private async executeInternalEventTrigger( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the naming here, createWorkflowEventListener
?
type: FieldMetadataType.TEXT, | ||
label: 'Name', | ||
description: 'The workflow event listener name', | ||
icon: 'IconPhoneCheck', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about the icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, but I did not find a better one. Since this is a system object, does it really matters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, but in that case, just remove icon: 'IconPhoneCheck',
, it is not mandatory
Basic endpoint that only returns a boolean currently and overrides the previous listener.